Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add recent_first alternative sorting arg to ImmudbClient.history() method #59

Open
wants to merge 1 commit into
base: feature/fullapisupport
Choose a base branch
from

Conversation

NickAnderegg
Copy link

@NickAnderegg NickAnderegg commented Oct 14, 2022

The current argument sortorder in ImmudbClient.history() is unclear from its name. Making it more confusing, it's the opposite of the typical Python convention for sort orders, where a boolean affecting sort order is typically False by default, e.g.:

sorted(iterable, /, *, key=None, reverse=False)

This change adds a new argument, recent_first, which has the same functionality as sortorder, but doesn't require reading the docs to understand what it means. I've implemented recent_first as a keyword-only argument and redefined sortorder as an optional positional-or-keyword argument.

This means that existing applications that call this method will be unaffected, but has the added advantage of still working as expected if someone gets confused about the argument names in the future. I've written it so that all of these calls would function identically:

client.history(key, offset, limit, True)
client.history(key, offset, limit, sortorder=True)
client.history(key, offset, limit, recent_first=True)
client.history(key, offset, limit, True, recent_first=True) # recent_first is ignored when sortorder is specified
client.history(key, offset, limit, sortorder=True, recent_first=True)
client.history(key, offset, limit, sortorder=True, recent_first=False)  # recent_first is ignored when sortorder is specified
client.history(key, offset, limit, sortorder=True, recent_first="xyz")  # recent_first is ignored when sortorder is specified

Because effectively specifying yes or no when the method asks what is our sort order? is very confusing!

@Razikus
Copy link
Collaborator

Razikus commented Oct 20, 2022

I would suggest to not add another parameter, just operate on the sortorder and make that statement in documentation

Adding new parameter, that actually only changes the other parameter is the same as renaming the first parameter, so maybe we should just rename sortorder

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants